What's the role of loadPerAccountDuration here? It doesn't look like anything else in the test refers to it.

Yeah it is not needed.

This test feels like it could easily stop having any effect if something otherwise unrelated changes about the timing of the steps here. It could end up not getting to the loading placeholder at all — or going past it, to the normal contents of HomePage.

How about having it check that it can find a widget with text containing, say, "taking a while"? That should be pretty identifying of the loading page.

We can't check for "taking a while" because the text cannot be rendered without the realmUrl. The fix replaced the page with SizedBox.shrink() which doesn't really stand out as a widget to look for. I ended up adding CircularProgressIndicator back instead. It might result in a layout shift as mentioned earlier.

Making the test easier to write is definitely not a good reason to change the actual UX in a way we previously decided we didn't like. 🙂

The SizedBox.shrink() only comes after logging out, right? The step I'm asking for above is to check that we got to the loading placeholder page in the first place, right before logging the user out.

Hmm, I see. Previously the account would finish loading before getting logged out. This setup relies on the behavior that after logging out, the app is briefly brought back to the loading page after removing the PerAccountStore and Account, and stays there until the route is popped from the navigator.

By removing the extra pump, checking it before logging out works.

Looking for another way to check for the alternative loading placeholder page after logging out, I found that we might do something like this without modifying the UX:

      // Missing AppBar title currently only applies to the loading pages,
      // but the check is still somewhat brittle.
      check(tester.widget<AppBar>(find.byType(AppBar))).title.isNull();

      check(tester.widget<SizedBox>(find.byType(SizedBox)))
        ..width.equals(0)
        ..height.equals(0);

For now, I'm pushing a revision that checks for the loading placeholder page before logging out, with the UX restored.

I see. Rereading #1219, it's really about what happens after the normal contents of HomePage have already loaded, and the account subsequently gets logged out. The involvement of _LoadingPlaceholderPage is really just a detail of how the bug happened — there's no reason there needs to be a "loading placeholder" involved in the process of removing an account.

OK, so in order to function as a regression test for that issue, the test should exercise that situation: the data fully loads, and then the account gets logged out. I think that's what an earlier revision of this PR (before last week) did.

But because the issue is about HomePage, not _LoadingPlaceholderPage, there's no need to have it check that a loading page is involved at that point… and it should go outside of the _LoadingPlaceholderPage test group. I think the test group is what confused me earlier — it's a way of saying the test is about _LoadingPlaceholderPage, so I noticed the test didn't really ensure _LoadingPlaceholderPage was involved in what it tested and that seemed like a problem.

In particular, consider what should happen if in the future we change the behavior of HomePage (perhaps via MaterialAccountWidgetRoute) so that when the account gets logged out, the loadingPlaceholderPage doesn't get built — instead it's just an empty page or something. This test will still be doing a useful job by checking that when the user gets logged out from HomePage, there's no crash — effectively checking that that new arrangement doesn't reintroduce this bug. But there's no need for it to complain about the fact that _LoadingPlaceholderPage is no longer present.

This new version of the test, where the loading page is still in place when the user gets logged out, is potentially useful too — so we might as well keep it, in addition to restoring the test that's a more direct regression test for #1219.